Add support for nested replacements inside format specifications #6616

zanieb · 2023-08-16T14:14:16Z

Python string formatting like "hello {place}".format(place="world") supports format specifications for replaced content such as "hello {place:>10}".format(place="world") which will align the text to the right in a container filled up to ten characters.

Ruff parses formatted strings into FormatParts each of which is either a Field (content in {...}) or a Literal (the normal content). Fields are parsed into name and format specifier sections (we'll ignore conversion specifiers for now).

There are a myriad of specifiers that can be used in a FormatSpec. Unfortunately for linters, the specifier values can be dynamically set. For example, "hello {place:{align}{width}}".format(place="world", align=">", width=10) and "hello {place:{fmt}}".format(place="world", fmt=">10") will yield the same string as before but variables can be used to determine the formatting. In this case, when parsing the format specifier we can't know what kind of specifier is being used as their meaning is determined by both position and value.

Ruff does not support nested replacements and our current data model does not support the concept. Here the data model is updated to support this concept, although linting of specifications with replacements will be inherently limited. We could split format specifications into two types, one without any replacements that we can perform lints with and one with replacements that we cannot inspect. However, it seems excessive to drop all parsing of format specifiers due to the presence of a replacement. Instead, I've opted to parse replacements eagerly and ignore their possible effect on other format specifiers. This will allow us to retain a simple interface for FormatSpec and most syntax checks. We may need to add some handling to relax errors if a replacement was seen previously.

It's worth noting that the nested replacement can also include a format specification although it may fail at runtime if you produce an invalid outer format specification. For example, "hello {place:{fmt:<2}}".format(place="world", fmt=">10") is valid so we need to represent each nested replacement as a full FormatPart.

Test plan

Adding unit tests for FormatSpec parsing and snapshots for PLE1300

crates/ruff_python_literal/src/format.rs

MichaReiser

Could you extend the PR summary with what parsing logic you changed? It is difficult to me to review this PR because I'm unfamiliar with format specifications and format.rs

crates/ruff_python_literal/src/format.rs

zanieb · 2023-08-16T14:28:49Z

Ya'll are too fast! I'm still working on this — I should have opened it as a draft but was in the process of writing up the body.

I added support for a replacement at the end of the specification (before the format type) but as Charlie has noted I've realized that we actually need to support any of the items in specification being a replacement. I'll need to consider a bit further the best way to accomplish this.

charliermarsh · 2023-08-16T14:30:09Z

We might need to just detect that the spec has nested parts, and return some other representation that isn't really analyzable by downstream rules (since we can't know how the spec will be resolved).

zanieb · 2023-08-16T16:24:07Z

crates/ruff_python_literal/src/format.rs

+        let replacements = RefCell::new(vec![]);
        // get_integer in CPython
+        let text = parse_nested_format_part(&replacements, text)?;


Would love suggestions here as this feels awkward.

An alternative, I guess, is to traverse the entire string twice. Once to collect and remove all of the replacements and a second pass on the cleaned text.

A bit tedious but I don't know that I have much better.

crates/ruff_python_literal/src/format.rs

zanieb · 2023-08-16T16:45:25Z

My decision to just extend FormatSpec may not work since all of these FormatSpec.format_... methods will drop replacements. I'm unsure of the best approach.

charliermarsh · 2023-08-16T16:45:56Z

What do you mean by "drop replacements"? Can you give an example?

zanieb · 2023-08-16T16:56:04Z

Sure! The following test will pass

        assert_eq!(
            FormatSpec::parse("{x}d")
                .unwrap()
                .format_int(&BigInt::from_bytes_be(Sign::Plus, b"\x10")),
            Ok("16".to_owned())
        );

but {x} could change the formatting and should not necessarily be ignored. Since all the format_... methods return results we could just return an error if replacements are present. I'm not sure where these methods are really used though.

zanieb · 2023-08-16T16:59:03Z

With e5d6ec3 I've updated PLE1300 to lint nested replacements as well.

charliermarsh · 2023-08-16T17:01:56Z

Ahhh I see, you're referring to the methods that actually use this to format things. Can we... remove those? I suspect they came from RustPython which needs to support these at runtime. Otherwise, I'd suggest returning an error in those cases.

github-actions · 2023-08-16T17:08:01Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      3.6±0.04ms    11.3 MB/sec    1.00      3.6±0.03ms    11.4 MB/sec
formatter/numpy/ctypeslib.py               1.00    736.1±7.99µs    22.6 MB/sec    1.00    733.6±3.11µs    22.7 MB/sec
formatter/numpy/globals.py                 1.00     75.9±0.42µs    38.9 MB/sec    1.01     77.0±1.59µs    38.3 MB/sec
formatter/pydantic/types.py                1.00  1454.5±13.98µs    17.5 MB/sec    1.00  1459.2±33.25µs    17.5 MB/sec
linter/all-rules/large/dataset.py          1.00     10.5±0.06ms     3.9 MB/sec    1.00     10.5±0.06ms     3.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.8±0.02ms     6.0 MB/sec    1.00      2.8±0.01ms     6.0 MB/sec
linter/all-rules/numpy/globals.py          1.01    397.9±0.94µs     7.4 MB/sec    1.00    394.5±2.81µs     7.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.4±0.10ms     4.7 MB/sec    1.00      5.4±0.09ms     4.7 MB/sec
linter/default-rules/large/dataset.py      1.00      5.5±0.02ms     7.3 MB/sec    1.01      5.6±0.03ms     7.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01  1227.4±44.65µs    13.6 MB/sec    1.00  1217.0±13.47µs    13.7 MB/sec
linter/default-rules/numpy/globals.py      1.00    141.0±2.59µs    20.9 MB/sec    1.04    145.9±6.01µs    20.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.01ms    10.3 MB/sec    1.00      2.5±0.02ms    10.2 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      4.2±0.10ms     9.7 MB/sec    1.00      4.2±0.08ms     9.7 MB/sec
formatter/numpy/ctypeslib.py               1.00   828.1±20.67µs    20.1 MB/sec    1.00   828.9±17.37µs    20.1 MB/sec
formatter/numpy/globals.py                 1.00     84.7±1.90µs    34.8 MB/sec    1.01     85.6±2.25µs    34.5 MB/sec
formatter/pydantic/types.py                1.00  1669.0±27.39µs    15.3 MB/sec    1.01  1685.2±25.52µs    15.1 MB/sec
linter/all-rules/large/dataset.py          1.00     12.7±0.13ms     3.2 MB/sec    1.05     13.3±0.21ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.04ms     4.8 MB/sec    1.03      3.6±0.06ms     4.6 MB/sec
linter/all-rules/numpy/globals.py          1.00    440.2±9.70µs     6.7 MB/sec    1.03   454.8±14.80µs     6.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.6±0.07ms     3.9 MB/sec    1.05      6.9±0.11ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      7.0±0.09ms     5.8 MB/sec    1.04      7.3±0.12ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1489.7±19.61µs    11.2 MB/sec    1.02  1523.9±27.75µs    10.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    176.5±4.46µs    16.7 MB/sec    1.02    180.4±3.75µs    16.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.06ms     8.1 MB/sec    1.01      3.2±0.04ms     8.0 MB/sec

zanieb · 2023-08-16T17:24:51Z

Great that works for me #6624

crates/ruff_python_literal/src/format.rs

zanieb · 2023-08-16T17:31:54Z

...les/pylint/snapshots/ruff__rules__pylint__tests__PLE1300_bad_string_format_character.py.snap

+20 | "{:{s:y}}".format("hello", s="s")  # [bad-format-character] (nested replacement format spec checked)
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLE1300


I'd really like better range information here. Would that be hard for me to add to FormatSpec?

It might be challenging because the thing we get on the left-hand side is the parsed representation which doesn't always match the literal representation. For example, the string on the left could be an implicit concatenation, or it could contain escaped characters, etc...

In #6616 we are adding support for nested replacements in format specifiers which makes actually formatting strings infeasible without a great deal of complexity. Since we're not using these functions (they just exist for runtime use in RustPython), we can just remove them.

crates/ruff_python_literal/src/format.rs

charliermarsh · 2023-08-16T18:03:38Z

crates/ruff_python_literal/src/format.rs

+        let format_part = FormatString::parse_part_in_brackets(&left)
+            .map_err(FormatSpecError::InvalidReplacement)?;
+        parts.push(format_part);
+        Ok(&right[1..])


right is guaranteed to be non-empty because the first character is the closing bracket?

I guess? Is split_at(idx + 1) clearer? I added a couple test cases and can't make it fail.

charliermarsh · 2023-08-16T18:05:11Z

crates/ruff_python_literal/src/format.rs

+            ],
+        });
+
+        assert_eq!(FormatString::from_str("abcd{1:{a}}"), expected);


Not for this PR, but I feel like these are good candidates for insta::assert_debug_snapshot, so that we don't have to write out and maintain the expectations manually.

Makes sense, I tried to match the existing pattern but I agree that's more maintainable.

crates/ruff_python_literal/src/format.rs

zanieb · 2023-08-16T19:47:47Z

crates/ruff_python_literal/src/format.rs

-    InvalidFormatSpecifier,
+    PlaceholderRecursionExceeded,


The former was only used to represent the latter which I found confusing and unnecessarily vague.

MichaReiser

Related to your question about better ranges. I have the impression that using Cursor instead of manually constructing strings and iterating over chars could simplify the implementation significantly (and speed it up!). We could even decide to build a small lexer that tokenized the strings, simplifying the "parsing" part.

…re present (#6858) Closes #6767 Replaces #6773 (this cherry-picks some parts from there) Alternative to the approach introduced in #6616 which added support for placeholders in format specifications while retaining parsing of other format specification parts. The idea is that if there are placeholders in a format specification we will not attempt to glean semantic meaning from the other parts of the format specification we'll just extract all of the placeholders ignoring other characters. The dynamic content of placeholders can drastically change the meaning of the format specification in ways unknowable by static analysis. This change prevents false analysis and will ensure safety if we build other rules on top of this at the cost of missing detection of some bad specifications. Minor note: I've use "replacements" and "placeholders" interchangeably but am trying to go with "placeholder" as I think it's a better term for the static analysis concept here

charliermarsh reviewed Aug 16, 2023

View reviewed changes

crates/ruff_python_literal/src/format.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Aug 16, 2023

View reviewed changes

crates/ruff_python_literal/src/format.rs Outdated Show resolved Hide resolved

crates/ruff_python_literal/src/format.rs Outdated Show resolved Hide resolved

zanieb marked this pull request as draft August 16, 2023 14:28

zanieb commented Aug 16, 2023

View reviewed changes

crates/ruff_python_literal/src/format.rs Show resolved Hide resolved

zanieb mentioned this pull request Aug 16, 2023

Remove unused runtime string formatting logic #6624

Merged

zanieb marked this pull request as ready for review August 16, 2023 17:24

zanieb commented Aug 16, 2023

View reviewed changes

crates/ruff_python_literal/src/format.rs Outdated Show resolved Hide resolved

zanieb commented Aug 16, 2023

View reviewed changes

charliermarsh reviewed Aug 16, 2023

View reviewed changes

crates/ruff_python_literal/src/format.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Aug 16, 2023

View reviewed changes

crates/ruff_python_literal/src/format.rs Outdated Show resolved Hide resolved

charliermarsh approved these changes Aug 16, 2023

View reviewed changes

zanieb added 6 commits August 16, 2023 14:27

Add support for nested replacements inside format specifications

9a00ec1

Improve test name

d8e4316

WIP: Refactor data structure

4331609

Refactor to parse _many_ replacements

d2234d6

Add note about nesting depth

a1b1289

Fix redundant closure

e8102a8

zanieb added 6 commits August 16, 2023 14:28

Add check to PLE1300 for nested format specs and snapshots

a987743

Clippy

5793d29

Drop RefCell usage

8e1527b

Move replacements access to method

964f49c

Refactor to reuse parse_spec implementation

c4dafba

Tweak right slice after split

b719975

zanieb force-pushed the fix/ple1300 branch from 4ca226b to b719975 Compare August 16, 2023 19:29

Cleanup after my sloppy rebase

60aa420

zanieb commented Aug 16, 2023

View reviewed changes

MichaReiser approved these changes Aug 17, 2023

View reviewed changes

zanieb merged commit d0f2a8e into main Aug 17, 2023

zanieb deleted the fix/ple1300 branch August 17, 2023 14:07

chenrui333 mentioned this pull request Aug 17, 2023

ruff 0.0.285 Homebrew/homebrew-core#139823

Merged

zanieb mentioned this pull request Aug 24, 2023

Avoid parsing other parts of a format specification if replacements are present #6858

Merged

renovate bot mentioned this pull request Oct 13, 2023

chore(deps): update all non-major dependencies nandhagk/sudoku#12

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for nested replacements inside format specifications #6616

Add support for nested replacements inside format specifications #6616

zanieb commented Aug 16, 2023 •

edited

Loading

MichaReiser left a comment

zanieb commented Aug 16, 2023

charliermarsh commented Aug 16, 2023

zanieb Aug 16, 2023

zanieb Aug 16, 2023

charliermarsh Aug 16, 2023

zanieb commented Aug 16, 2023

charliermarsh commented Aug 16, 2023

zanieb commented Aug 16, 2023

zanieb commented Aug 16, 2023 •

edited

Loading

charliermarsh commented Aug 16, 2023

github-actions bot commented Aug 16, 2023 •

edited

Loading

zanieb commented Aug 16, 2023

zanieb Aug 16, 2023

charliermarsh Aug 16, 2023

charliermarsh Aug 16, 2023

zanieb Aug 16, 2023

charliermarsh Aug 16, 2023

zanieb Aug 16, 2023

zanieb Aug 16, 2023

MichaReiser left a comment

		20 \| "{:{s:y}}".format("hello", s="s") # [bad-format-character] (nested replacement format spec checked)
		\| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLE1300

Add support for nested replacements inside format specifications #6616

Add support for nested replacements inside format specifications #6616

Conversation

zanieb commented Aug 16, 2023 • edited Loading

Test plan

MichaReiser left a comment

Choose a reason for hiding this comment

zanieb commented Aug 16, 2023

charliermarsh commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb commented Aug 16, 2023

charliermarsh commented Aug 16, 2023

zanieb commented Aug 16, 2023

zanieb commented Aug 16, 2023 • edited Loading

charliermarsh commented Aug 16, 2023

github-actions bot commented Aug 16, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

zanieb commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

zanieb commented Aug 16, 2023 •

edited

Loading

zanieb commented Aug 16, 2023 •

edited

Loading

github-actions bot commented Aug 16, 2023 •

edited

Loading